Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround for ICC profile parsing bug in Qt #683

Merged
merged 2 commits into from
Jun 25, 2024

Conversation

jdpurcell
Copy link
Contributor

Fixes #673. QColorSpace, even if considered invalid, preserves the original ICC profile data (except for PNG ☹️), which makes this possible. An oversight in Qt's ICC parser makes it require that all data chunks are at least 12 bytes, but per the ICC spec, some can be smaller, like a text field (presumably for something unimportant like a comment) with no or only a few characters. This workaround rewrites the ICC profile's table of contents to remove references to any data segments less than 12 bytes. See page 20 (labeled page 11 in the footer) of the ICC specification for a brief overview of the file format.

@jurplel
Copy link
Owner

jurplel commented Jun 25, 2024

Thanks for the PR!

I will trust that all works since I don't think I have time to test it myself on all platforms.

Side discussion:
It really seems to me that Qt's color space implementation is kind of weak. I feel that a smart way to go forward with this application would be integrating Magick++, configured to load littlecms. This would completely eliminate the annoying image plugin build process, give us more image format support that is less buggy, and probably give us extra features in the process too (maybe HDR support for EXR images? I am investigating this)

This would give us like one dependency to load but it would be less hassle than what we're going through now for these plugins.

I would definitely be open to working on a design plan for this conversion, testing it, and accepting a PR—let me know your thoughts :)

@jurplel jurplel merged commit 34311df into jurplel:master Jun 25, 2024
6 checks passed
@hogsterbogster
Copy link

Thanks for fixing this issue!

Where can I download the fixed version of the program for the Mac please, as this download doesn't seem to have been updated since August 2023?

Many thanks,

David

@jdpurcell
Copy link
Contributor Author

jdpurcell commented Jun 27, 2024

@hogsterbogster There are recent builds from GitHub Actions; the one I'd recommend for macOS currently can be downloaded here.

@jurplel I'd say I'm indifferent. I mean if someone has the time and motivation and sees a particular problem that it solves, sure. I'm mostly using qView to view simple jpg files taken with a phone camera, meme pics, etc.

I haven't seen too much issue with the color space support aside from the few issues I already fixed / worked around, CMYK which is already working in Qt 6.8 beta, and whatever's going on in #660 which I'm not really sure about. On that last point, there might be some slight degradation due to color space conversion rounding errors, which can already be worked around by converting the format to Format_RGBA32FPx4 prior to conversion (or in 6.8, convertToColorSpace gets a new overload that can do the conversion inline). Even with that it still looks different than Preview, but it at least matches Safari, so I'm not sure if Safari doesn't fully support 16-bit images, or Preview has some post-processing magic, or what.

As I don't have any computers with HDR displays currently, I can't say I have a particular interest in that, but maybe in a few years if I get a M5 MacBook Pro with OLED 😄.

And I can't speak to the build process. I wouldn't know where to start trying to build to it or how to link to it. Before working on qView I had fairly limited experience with C++ and I've never made anything but the simplest C++ program from scratch, all self-contained not using any 3rd party libraries. So I've mostly just been tinkering with the build systems you already put in place. I guess it's unclear to me how Magick would simplify the build process, would you not need to build it (and any other dependencies if it has any) just like KImageFormats, i.e. you're just trading one heap of 3rd party code for another?

@jurplel
Copy link
Owner

jurplel commented Jun 27, 2024

you're just trading one heap of 3rd party code for another?

In essence yes, but it would be one well supported vcpkg instead of a matrix of kinda-supported image format plugins.

I haven't seen too much issue with the color space support aside from the few issues I already fixed / worked around

That's good. In that case htis might not be necessary.

I'm mostly using qView to view simple jpg files taken with a phone camera, meme pics, etc.

That's what I had in mind when I created it as well, so perhaps it is best not to let the scope expand that much.

@jdpurcell jdpurcell deleted the issue673 branch June 29, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Colour space not rendering correctly?
3 participants